Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from indutny/elliptic to noble-secp256k1. #727

Closed
wants to merge 2 commits into from
Closed

Switch from indutny/elliptic to noble-secp256k1. #727

wants to merge 2 commits into from

Conversation

paulmillr
Copy link

@paulmillr paulmillr commented Feb 7, 2020

I've been developing a highly auditable self-contained zero-dependency secp256k1 library @ https://github.com/paulmillr/noble-secp256k1 & https://paulmillr.com/posts/noble-secp256k1-fast-ecc/

The PR should also resolve #666, since noble-secp256k1 is algorithmically protected against timing attack.

The library will increase security of ethers, since all Noble releases are signed, don't contain dependencies and a user is able to verify the source code in a simpler way versus indutny/elliptic, which uses BigNum library with thousands of LOC.

It also has very simple API and is fun to play with.

A hacker could break into any dependency of elliptic and steal important user data. This has happened before:

@paulmillr paulmillr requested a review from ricmoo February 7, 2020 11:15
@ricmoo
Copy link
Member

ricmoo commented Feb 7, 2020

Looks awesome!

Unfortunately it looks like it is using the JavaScript bigint which I don’t plan to switch to until v6. There are still a lot of platforms which do not support it yet. :(

@paulmillr
Copy link
Author

@ricmoo which platforms specifically are you targetting?

@paulmillr
Copy link
Author

All modern platforms except for Safari support bigints. So, the userbase is quite high today.

  • Chrome, Firefox, Edge, nodejs 10, nodejs12 support bigints
  • Safari doesn't support bigint, that's sad. It should have the feature in a couple months.
  • nodejs 8 doesn't support bigint. The version is officially unsupported today. We don't want folks to use nodejs which doesn't receive security updates, do we?

Even if someone wants to use nodejs6-8, they could use v4 instead of v5.

@ricmoo
Copy link
Member

ricmoo commented Feb 7, 2020

Here is a useful table to track the feature, but here is a quick list of things that matter more to me:

  • DukTape
  • Node 8 (which affects a lot of other environments, like electron, react-native, expo)
  • Safari iOS (so no iPhones would support it; eek! That's me!)
  • Safari macOS (so I won't be able to develop it ;))
  • Apparently Edge doesn't support it; while I don't target it directly, it seems abrasive to drop support for them
  • Otto; doesn't matter to me a lot, but I know it is used by ethers users, because I hear about it when I use any look-ahead regex (which it doesn't support)

This also only represents the direct versions, keep in mind that many applications may be using even slightly out-of-date versions of react-native or electron, which might be using slightly more out-of-date node, and so on, which forces these other projects to update.

I plan to have the next major version move to more modern things (BigInt is still undecided), but Proxy and WeakMap are things I'm very excited to add support for.

Make sense?

@paulmillr
Copy link
Author

paulmillr commented Feb 7, 2020

@ricmoo I agree we should delay this until Safari supports bigints, but for officially unsupported nodejs v8: users need to upgrade, period. It's a security issue, like, for real. There are electron versions with modern / supported nodejs bundle.

@ricmoo
Copy link
Member

ricmoo commented Feb 7, 2020

As someone who maintains a lot of random apps and projects, dropping support is a huge hassle. Often you can't just upgrade a single library. If a library stops working on node 8, and you are based on an older react-native app, you then upgrade node, which breaks other libraries which have other incompatibilities, and possibly have to re-write part of your main app, because now the react-native API has changed and so on. Node 8 only ended maintenance mode a month ago, and there is a lot of catch-up that is always needed.

I will probably make that decision in v6, that in order to support older platforms you will need to continue using v5 (which means I need to continue bug fixes in a legacy version :s). But v5 is too close to release and I did not give any advanced warning that I would drop the user base at all, certainly not "all apple devices" (including, I'm guessing JavaScripCore, which I'm planning to use in ethers wallet in the next version).

Again, I speak as someone who has had to deal with these events often. I stopped using Cocoa Pods as a result of the packages becoming so legacy, so quickly and instantly dissolving the entire dependency tree to the point there is no spanning tree that produces a working set of libraries, but C has additional complications of symbol collisions and weird mutually-exclusive compile time flags different people depend on. :p

Luckily (?), this is more of a headache for me than anyone else, since consumers of ethers don't need to know what is happening inside. This is one reason why ethers wraps BN.js, so that it is easy to swap it out if I ever change secp256k1 libraries.

I also apologize if I sounded a bit defensive, but this is an discussion I've had many times for a lot of various language features and design patterns for both JavaScript and TypeScript.

Oh! But I should point out, if you are on a platform that has BigInt, and you pass it into v5, it will accept it fine as an input value. :)

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Feb 7, 2020
@paulmillr
Copy link
Author

Sounds great!

@paulmillr
Copy link
Author

I've wrote a blog post about the library. Basically during march i've made it faster than indutny/elliptic. Check it out: https://paulmillr.com/posts/noble-secp256k1-fast-ecc/

@ricmoo
Copy link
Member

ricmoo commented May 9, 2020

Awesome! I’d love to use it, but it looks like it still uses JavaScript BigInt?

Also great article. I read through it quickly, but am looking forward to reading through it more closely when I get a moment. :)

@paulmillr
Copy link
Author

Yup, I don't see any other option that doesn't requires thousands of lines of code re-implementing the BigInt functionality for Apple Safari.

After all, it's just safari that stops us from merging this. all major platforms have supported this for at least a couple releases. All stable node.js versions also.

@paulmillr
Copy link
Author

Kudos -- Safari update would support BigInt this year, in a few months. Webkit already merged necessary changes. This would allow us to support all browsers.

@ricmoo
Copy link
Member

ricmoo commented Jun 15, 2020

I will still probably wait a bit longer to make sure there is fairly substantial support, but I’ve written a Babel plugin that can convert your TypeScript library into BigNumber-object-based TypeScript at build-time, so I’m hoping I can switch to it earlier and have extra dist files for es2020, which gains pretty substantial file size improvements for anyone who doesn’t mind dropping support for slightly older browsers. :)

The plugin right now is fairly specific to your secp256k1 library, but I’m trying to make it a bit more generic to support your bls library too. :)

@paulmillr
Copy link
Author

Wow, sounds awesome!

With the plugin, besides native support @ Chrome+Firefox+Safari+Edge+Node, we'll cover all other browsers!

@paulmillr
Copy link
Author

Let me know if there are some hard points which could be changed in bls12-381 to make babel parsing effortless.

@paulmillr
Copy link
Author

@ricmoo heads up — all major browsers including iOS / Macos safari support bigints out-of-box.

@paulmillr
Copy link
Author

Bigints are 88.72%-supported right now: https://caniuse.com/bigint

@paulmillr
Copy link
Author

I see you've changed directory structure. Here's new signing-key/src.ts/index.ts converted to noble: https://gist.github.com/paulmillr/b37bbf3c2ab19f610862fd398f5597e9

@nissoh
Copy link

nissoh commented Jun 3, 2021

will be nice to see a switch from BigNumber into BigInt as its more readable, compatible and future proof. thank you for pushing this @paulmillr

@ajb
Copy link

ajb commented Jul 19, 2021

Thank you both for working on this. Any plans to still implement?

@ricmoo
Copy link
Member

ricmoo commented Jul 19, 2021

Definitely! But it likely cannot go into v5.

This library depends on BigInt, which is a fairly recent addition to the JavaScript language. It was only adopted by JavaScriptCore last September, which means any dependency based on JavaScriptCore would instantly abort if trying to load it (as it is a syntax error), not to mention it wouldn't work anyways.

Safari for iOS and macOS both use JavaScriptCore, which means on those platforms it would fail. Most users of iOS and macOS remain up to date, so that is of less concern than React Native, which is built on-top of JavaScriptCore, along with Expo and many other "forks".

This means that for RN and all those forks and tweaks of RN, any developer that depends on ethers would be forced to upgrade their underlying framework, which is always a huge undertaking. As a maintainer of several legacy packages, it is debilitating when a dependency requires such a dramatic change. I have suffered at the hand of many such changes, so don't plan to inflict that on anyone myself. :)

In v6, I have plans to use Noble by default, but to also have the build scripts dump out a dist/ethers.compat.umd.js and dist/ethers.compat.umd.min.js which will shim in an elliptic-based (probably?) implementation for those requiring a higher level of compatibility with older systems.

I've been spending a lot of time on v6, and it's coming along. :)

@ajb
Copy link

ajb commented Jul 19, 2021 via email

@paulmillr
Copy link
Author

I've just pushed _syncSign() support via paulmillr/noble-secp256k1@5ce349b, released in 1.2.8.

That was a requirement for ethers to include noble, see twitter thread how we've been resolving it: https://twitter.com/ricmoo/status/1417050092759101440

@paulmillr
Copy link
Author

Just rebased this 15month-old pull request with master and it's ready to be merged @ricmoo

I did not remove elliptic and bn.

@ajb
Copy link

ajb commented Jul 22, 2021

I just want thank you both again prematurely, as I checked out the code and can confirm that for my implementation, it indeed speeds things up by a factor of 2x. :)

@paulmillr
Copy link
Author

I've changed the API to syncSign and utils.hmacSha256Sync

@paulmillr
Copy link
Author

The official ethereum-cryptography package is now using noble-secp256k1; 15 times smaller; depending on 3 packages instead of 38.

Makes sense to ditch those in favor of it:

    "bech32": "1.1.4",
    "bn.js": "^4.11.9",
    "elliptic": "6.5.4",
    "hash.js": "1.1.7",
    "js-sha3": "0.8.0",
    "scrypt-js": "3.0.1",

@ricmoo
Copy link
Member

ricmoo commented Oct 20, 2021

I am using them in v6. But I'm making as few and as small changes to v5 as possible. :)

I was glad to see you used a similar API for your scrypt library as I do in js-scrypt. That was a hard requirement, and I'll likely use your solution over mine. ;)

@paulmillr
Copy link
Author

@ricmoo onProgress is awesome, thanks for the inspiration!

@ricmoo
Copy link
Member

ricmoo commented May 14, 2022

FYI. This is used in v6. Thanks for the awesome libraries!! :)

Closing this PR now. :)

@ricmoo ricmoo closed this May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, feedback and general information.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update elliptic to protect from Minerva Timing Attack
4 participants